Conversation
Three fixes: 1. Fix race condition on sessions_ map - accept_task_function now locks session_mutex_ when modifying sessions_, matching the locking in session_task_function that iterates/erases sessions. 2. Teardown session after failed SETUP - when TCP transport is rejected (or any SETUP parse failure), the session is properly torn down so it gets cleaned up instead of remaining as a zombie. 3. Include CSeq in 461 response - the "Unsupported Transport" response now includes the sequence number per RTSP protocol. Co-authored-by: finger563 <213467+finger563@users.noreply.github.com>
Co-authored-by: finger563 <213467+finger563@users.noreply.github.com>
Co-authored-by: finger563 <213467+finger563@users.noreply.github.com>
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical crash in the RTSP server that occurs when clients request TCP transport (e.g., Home Assistant connections). The server was becoming permanently unresponsive due to a race condition and zombie sessions.
Changes:
- Fixed data race on
sessions_map by adding mutex protection during session insertion - Added session teardown on failed SETUP requests to prevent zombie sessions
- Fixed RTSP protocol violation by including CSeq header in 461 "Unsupported Transport" responses
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| components/rtsp/src/rtsp_server.cpp | Added mutex lock guard around sessions_.emplace() to fix race condition between accept_task and session_task threads |
| components/rtsp/src/rtsp_session.cpp | Added teardown() call on SETUP failure and included CSeq in TCP transport rejection response |
| logger_.error("TCP transport is not supported"); | ||
| // TODO: this doesn't send the sequence number back to the client | ||
| send_response(461, "Unsupported Transport"); | ||
| handle_rtsp_invalid_request(request, 461, "Unsupported Transport"); |
There was a problem hiding this comment.
The magic number 461 should be defined as a named constant (e.g., RTSP_UNSUPPORTED_TRANSPORT = 461) to improve code readability and maintainability.
| /// Handle an invalid RTSP request | ||
| /// @param request The request to handle | ||
| /// @param code The response code to send (default: 400) | ||
| /// @param message The response message to send (default: "Bad Request") | ||
| /// @return True if the request was handled successfully, false otherwise |
There was a problem hiding this comment.
The function documentation should be updated to explain that this method now handles both generic invalid requests and specific protocol errors, and clarify when custom codes/messages should be used versus defaults.
| /// Handle an invalid RTSP request | |
| /// @param request The request to handle | |
| /// @param code The response code to send (default: 400) | |
| /// @param message The response message to send (default: "Bad Request") | |
| /// @return True if the request was handled successfully, false otherwise | |
| /// Handle an invalid RTSP request. | |
| /// | |
| /// This helper is used both for generic invalid requests (for example, malformed | |
| /// or incomplete RTSP messages) and for specific protocol or semantic errors | |
| /// detected while parsing or validating a request. | |
| /// | |
| /// If no explicit status is provided, a generic `400 Bad Request` response is | |
| /// sent to the client. | |
| /// | |
| /// @param request The request to handle. | |
| /// @param code The RTSP status code to send. Use the default (400) for generic | |
| /// invalid requests, and provide a more specific code (for example, | |
| /// 401, 404, 405, or 454) when reporting a particular protocol error. | |
| /// @param message The RTSP reason phrase to send. Defaults to "Bad Request" and | |
| /// should normally be overridden when sending a custom @p code | |
| /// to better describe the specific error condition. | |
| /// @return True if the request was handled successfully, false otherwise. |
RTSP server becomes permanently unresponsive after a client connects requesting TCP interleaved transport (e.g., Home Assistant). No subsequent clients can connect until reboot.
Changes
Fix data race on
sessions_map —accept_task_functionwas mutatingsessions_without holdingsession_mutex_, whilesession_task_functioniterates/erases under that same mutex on a different thread. Added lock guard around theemplace.Teardown session on failed SETUP — After rejecting TCP transport with 461, the session was left alive (
closed_=false,session_active_=false) as a zombie. Now callsteardown()sosession_task_functioncleans it up on the next iteration.Include CSeq in 461 response — The "Unsupported Transport" response was missing the CSeq header, violating RTSP protocol.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.